upb: validate wire type before promoting an unknown field to a message extension#28144
Open
kuranikaran wants to merge 1 commit into
Open
upb: validate wire type before promoting an unknown field to a message extension#28144kuranikaran wants to merge 1 commit into
kuranikaran wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
upb_Message_GetOrPromoteExtension()matches a message's unknown fields by field number only. A same-number unknown carrying a non-delimited wire type (varint or fixed) was still captured and handed toupb_MiniTable_ParseUnknownMessage(), which re-decodes the bytes as a length-delimited sub-message.ParseUnknownMessage()reads a tag and then a varint length from those bytes:For a varint field such as
08 01(field 1, value 1), the value1is consumed asmessage_lenanddatais left one byte past the end of the captured field, soupb_Decode()reads out of bounds.This adds a
kUpb_WireType_Delimitedcheck to the match. A same-number field with any other wire type now falls through to the existing_upb_WireReader_SkipValue()branch and is left as an unknown, exactly like an unrelated field.Tests
Added
PromoteIgnoresWrongWireType:ModelExtension1.model_extencoded with a varint wire type now returnskUpb_GetExtension_NotPresentinstead of being promoted.Also verified with a standalone
-fsanitize=address,undefinedbuild: the wrong-wire input no longer faults and returnskUpb_GetExtension_NotPresent, and a correctly length-delimited empty-message extension still promotes (kUpb_GetExtension_Ok, non-null message).Follow-up
The legacy
PromoteUnknownToMessage/...Array/...Mappaths reach the sameParseUnknownMessage()sink via the publicupb_Message_FindUnknown(). I left that helper untouched to avoid changing its semantics; the same guard can be applied there in a follow-up if you'd prefer.